Skip to content

Ecoclient Simvue Python API Extension #752

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 41 commits into from
Mar 28, 2025
Merged

Ecoclient Simvue Python API Extension #752

merged 41 commits into from
Mar 28, 2025

Conversation

kzscisoft
Copy link
Collaborator

@kzscisoft kzscisoft commented Mar 7, 2025 •
edited
Loading

Ecoclient

Issue: #751

Python Version(s) Tested: 3.13

Operating System(s): Ubuntu 24.10

Documentation PR: https://github.com/simvue-io/docs/pull/61

📝 Summary

Replaces CodeCarbon as the source of emission metrics giving measures for CO2 emissions and energy consumption.

🔄 Changes

  • Removed CodeCarbon based EmissionsTracker from simvue.Run.
  • Created eco submodule containing:
    • APIClient: Class for connecting to CO2 Signal API.
    • CO2Monitor: Class for calculating values based on CPU usage and current CO2 intensity.
  • Integrated CO2Monitor into simvue.Run and attached metric gathering to heartbeat (similar to resource metrics).

✔️ Checklist

  • Unit and integration tests passing.
  • Pre-commit hooks passing.
  • Quality checks passing.
  • Updated the documentation.

@kzscisoft kzscisoft added enhancement New feature or request improvement labels Mar 7, 2025
@kzscisoft kzscisoft linked an issue Mar 7, 2025 that may be closed by this pull request
Replace CodeCarbon with simpler API #751
Closed
@kzscisoft kzscisoft added this to the v2.1.0-rc1 milestone Mar 7, 2025
@kzscisoft kzscisoft marked this pull request as ready for review March 13, 2025 09:44
@wk9874
Copy link
Collaborator

wk9874 commented Mar 13, 2025

Initial comments:

  • Unit test is failing, please correct
  • I think you should raise a docs MR which contains updated information on how EcoClient works, a warning that it is an early version and the values aren't asbolute, etc

AbyAbraham21
AbyAbraham21 reviewed Mar 17, 2025
View reviewed changes
AbyAbraham21
AbyAbraham21 reviewed Mar 17, 2025
View reviewed changes
AbyAbraham21
AbyAbraham21 reviewed Mar 17, 2025
View reviewed changes
AbyAbraham21
AbyAbraham21 reviewed Mar 17, 2025
View reviewed changes
Copy link
Collaborator

@AbyAbraham21 AbyAbraham21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calculations look right. Some comments to address regarding offline mode.

wk9874
wk9874 requested changes Mar 17, 2025
View reviewed changes
simvue/run.py Outdated
self._emissions_tracker: OfflineSimvueEmissionsTracker | None = (
OfflineSimvueEmissionsTracker(
"simvue", self, self._emission_metrics_interval
if not (_co2_intensity := self._user_config.eco.co2_intensity):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should probably not require this, and should do the same as CC where Simvue comes with some default file which it can read from (that we make sure to update each PyPI release). That way the user just needs to provide a ISO code, not a value.

We could also make it so that the sender makes a call to the carbon intensity data every (eg) hour, and records it in the same file which online mode produces.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would argue in doing this we are then held accountable for this value.

simvue/run.py
elif enable_emission_metrics is False and self._emissions_tracker:
self._error("Cannot disable emissions tracker once it has been started")
elif enable_emission_metrics is False and self._emissions_monitor:
self._error("Cannot disable emissions monitor once it has been started")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this is a remnant of CC when this was not possible due to CC starting a server of sorts.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cannot disable because once the loop is running you cannot modify it

@wk9874
Copy link
Collaborator

wk9874 commented Mar 17, 2025

Final general comment - I strongly think that we should be adding in contributions from GPUs into this initial version of this if not too much work. Other stuff like RAM etc doesnt matter so much as it'd be roughly constant between runs, but if you did a run on a GPU cluster and a run on a CPU cluster, our emissions metrics comparison between the two would basically just be lies :/

@wk9874
Copy link
Collaborator

wk9874 commented Mar 19, 2025

Would suggest making emissions and resource metrics interval the same, and then doing something like:

In run.py, in _heartbeat:

                with self._configuration_lock:
                    if (
                        self._resources_metrics_interval
                        and (res_time := time.time()) - last_res_metric_call
                        > self._resources_metrics_interval
                    ):
                        # Set join on fail to false as if an error is thrown
                        # join would be called on this thread and a thread cannot
                        # join itself!
                        _sysinfo = self._get_sysinfo()
                        self._add_metrics_to_dispatch(
                            _sysinfo, join_on_fail=False, step=res_step
                        )
                        if emissions:
                            <calculate emissions based on _sysinfo["cpu"]>
                        last_res_metric_call = res_time
                        res_step += 1

@kzscisoft kzscisoft requested a review from wk9874 March 20, 2025 16:54
wk9874
wk9874 approved these changes Mar 28, 2025
View reviewed changes
simvue/config/parameters.py Outdated
r"^(\/|([a-zA-Z]:\\))?([\w\s.-]+[\\/])*[\w\s.-]*$", f"{cache}"
):
raise AssertionError(f"Value '{cache}' is not a valid cache path.")
return cache


class MetricsSpecifications(pydantic.BaseModel):
resources_metrics_interval: pydantic.PositiveInt | None = -1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would call this 'system_metrics_interval' as it now applies to both resources and emissions metrics

simvue/eco/emissions_monitor.py Outdated
)

if not isinstance(kwargs.get("thermal_design_power_per_gpu"), float):
kwargs["thermal_design_power_per_gpu"] = 80.0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change to 130

@kzscisoft kzscisoft merged commit 8f13a7a into dev Mar 28, 2025
14 of 17 checks passed
@kzscisoft kzscisoft deleted the feature/ecoclient branch March 28, 2025 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AbyAbraham21 AbyAbraham21 AbyAbraham21 left review comments

@wk9874 wk9874 wk9874 approved these changes

Assignees

@kzscisoft kzscisoft

Labels
enhancement New feature or request improvement
Projects
None yet
Milestone
v2.1.0-rc1
Development

Successfully merging this pull request may close these issues.

Replace CodeCarbon with simpler API
3 participants
@kzscisoft @wk9874 @AbyAbraham21